-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Require object types to have fields and input objects to have arguments #5137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
FWIW: "use cases" is legitimate. We use ActiveSupport load hooks to assemble root scopes from across the codebase, which means root scopes start as empty object types. So, don't write |
|
Thanks for sharing that, @gmac. I would expect this assertion to run during the first query that the schema executes, and by that time, any object type being used would have had its fields loaded. Is that right? Or did you find that this raised errors for you before those hooks ran? If you did encounter errors, I'd be interested to see the backtrace. My goal is for this to halt on execution or dumping the schema, but for it to not halt on any boot-time steps. |
|
I think this is more on us than GraphQL Ruby; our approach is pretty novel. Our mutation root now basically looks like this: class MutationRoot < GraphQL::Schema::Object
graphql_name "Mutation"
if Rails.configuration.eager_load
Hooks.run!
else
# allow no fields when running lazy load hooks
has_no_fields(true)
end
endThen, across the codebase are load hooks that attach fields to an injected mutation object, ie: Hooks.configure_mutation_root do |m|
m.field :do_stuff, true
endSo basically, we have to load |
|
Sure, I haven't heard of anyone using hooks to build the schema, but it should generally be supported without errors. Ruby classes being open, etc, everything is supposed to be open to extension at roughly any time. I'd be interested in seeing the backtrace on the error that this raised in your app, even just the GraphQL-Ruby parts, so I could get a sense for why it didn't work in this case. Maybe it's because something in The intent of the implementation here was to allow objects without fields until the object is used in a query. Because as described here, it's reasonably to have object definitions whose fields haven't been added yet. But I don't know of a use case for a object type with no fields at query time! |
|
Oh, I see what you’re saying. I’ll turn it off and see where the error happened. It was specific to tests with mutations. Sounds like dev env runs JIT hooks as part of the query itself; which wouldn’t surprise me with the aggressive startup optimizations that a dedicated team is making across the codebase. |
GraphQL doesn’t support empty objects `{}` but the special_route jsonnet
schema forces that to be the value of the `details` field, and the
frontend schemas derived from jsonnet require the field to exist.
* `special_route` jsonnet specifies details: "forbidden". For some
reason, `GovukSchemas::Schema.find(frontend_schema: "special_route")`
still thinks that the `details` field is required as
`"details" => {"type" => "object", "additionalProperties" => false, "properties" => {}}`
https://github.com/alphagov/publishing-api/blob/5173e32cb1e3dc446dcb45f30e00ae6119d47ebf/content_schemas/formats/special_route.jsonnet#L4)
* `/lib/schema_generator/format.rb` defines `"forbidden"` to be `{}`
https://github.com/alphagov/publishing-api/blob/5173e32cb1e3dc446dcb45f30e00ae6119d47ebf/content_schemas/formats/special_route.jsonnet#L4
* GraphQL spec: https://spec.graphql.org/October2021/#sel-FAHZhCFDBAACDA4qe
* Ruby implementation: rmosolgo/graphql-ruby#5137
This commit patches the ContentItemCompactor to create an empty
`details` field when it is 'required'. This occurs after graphql has
returned the content item, but before the tests that check that the
output schema has been matched.
The spec says that object types must have fields and that input object types must have arguments. This change adds that requirement to query execution and runtime. (Since Ruby classes are open, there's not a "compile-time" or "build-time" to check the definition.)
This change keeps the current behavior when all fields are hidden by
.visible?checks -- the type is still accessible but all fields are hidden. It might be possible to raise an error in that case but I don't think it's worth it.For backwards compatibility (or ... use cases ?), there's
has_no_fields(true)andhas_no_arguments(true). Adding these configs will silence errors from GraphQL-Ruby.Also,
RelayClassicMutation(andHasSingleInputArgument) will still create empty input objects by default, withhas_no_arguments(true)called on them. This is how it has always worked and I don't want to 💥 existing users 😅Fixes #4509
TODO